Release 4.3.0 - #265
Conversation
`is_known_good` was `state == 'KNOWN_GOOD' or bool(known_good_sources)`, and the Detections line it drives replaces the engine verdicts with "this artifact is a known-good binary … it is not scanned". That was harmless only because the server emitted the feed list exclusively for instances already in the known-good state. It no longer does: the list is now present whenever the sha256 matches a known-good record, including on a fully scanned instance with real detections and a PolyScore, so the fallback would have reported such a sample as never scanned. The state is the single reliable signal for "known-good, bytes withheld", so it alone decides; the feed list only shapes the message. Keeps the defensive getattr for older SDKs — one consequence, now documented: an SDK without `.state` renders a known-good instance as an ordinary one rather than mislabelling a scanned one.
…tate
The change made `.state` load-bearing with no fallback, but the dependency floor
still allowed 4.0.0, which predates it — so a user on an in-range SDK would get a
known-good instance rendered as an ordinary window-closed one ("No engines
responded… trigger a rescan"), silently. `.state` shipped in 4.1.0, and the SDK
contract says the floor must exclude releases missing behaviour the CLI relies on.
Also: specs/04-testing.md's invariant said tests drive the CLI through CliRunner
and never call internal functions, which as written forbade the formatter-unit
tests this change adds — the spec now sanctions that style and says when it is the
right choice. specs/03-formatters.md no longer overstates the feed-list guard as
load-bearing. And two branches where the flag has to lose or be ignored are now
pinned: a failed known-good instance reports the failure, and an instance with
feeds but no state renders as an ordinary one — the assertion that would catch the
removed fallback being reintroduced.
…ample is unscanned Two behaviours the CLI relies on arrived in 4.2.0, not 4.1.0: llm_report_create only started sending the client's community then (and `report llm-create` does not pass one itself, so on 4.1.0 a private-community report is created without it), and streaming downloads only started raising NoResultsException on a 204 then, which the download command's no-results exit code depends on. The assertion loop was not gated by the known-good flag, so an instance carrying that state *and* assertions rendered "it is not scanned" immediately followed by per-engine verdicts. That pairing is no longer hypothetical: the server now reconciles a previously scanned instance into the known-good state while deliberately preserving its assertions, polyscore and detections, so it is exactly what a client receives. The rendering now says both things without contradicting itself — the bytes are withheld, and here are the results we already had.
Review follow-ups on the known-good rendering: - A known-good instance carrying malicious detections rendered the whole Detections line green, including the "N/M engines reported malicious" clause the ordinary branch renders red. Green on 40/50 is a weaker warning than the same instance gave before it was reconciled, which is the class of mis-signal this rendering exists to fix. Red when there are malicious assertions and the window is closed; green otherwise. - The counts are now guarded on window_closed like every other branch, so an open window's numbers are never presented as final. - Pin the colour decision against TextOutput(color=True): every existing test unstyles its output, so the green/red choice was unobservable. - Drop the `del instance.state` half of the no-state test — it mutates an SDK resource's internals, which specs/05 forbids, and pins a configuration specs/03 declares unsupported. The server-omits-state case it shared covers the real behaviour. - Document the floor's two preconditions (published on PyPI, and declared by the SDK's develop archive) — both verified for 4.2.0.
…t goes red specs/03 states it as a rule; _detections_line() filters the styled render down to the Detections line, so nothing observed it.
Review follow-ups. `self.color` was assigned twice in __init__ and read nowhere: every _white/_red/_green helper called click.style() unconditionally, so `--no-color` was a no-op for text output while JSONOutput honoured it and specs/03 claimed TextOutput did too. Masked in practice because click strips ANSI when stdout is not a tty. All five helpers now go through one `_paint`, which is the only place the flag is read. That also corrects what this PR's own colour tests pin: it is the absent click.unstyle, not the `color=` argument, that made the green/red decision observable. The spec and the helper docstring said otherwise. Also: - The spec header said "majority-malicious" while the code reddens on any malicious assertion — the same threshold the ordinary branch uses. - Pin the open-window colour. That is the one combination where the `and window_closed` conjunct is load-bearing: the instance has a malicious assertion, so dropping it turns the line red while it reads "its scan has not finished running yet", and every existing test still passed.
…inned `state` is load-bearing with no fallback, and a Style-3 formatter test cannot see the transport boundary. Rather than leave that inferred: the key and the label were read off the server's ArtifactInstanceSerializer (`'state': instance.state.name`, and BountyState.KNOWN_GOOD.name is exactly 'KNOWN_GOOD'), and the thing that would catch a future rename is the server's own suite — this repo replays frozen cassettes with no VCR-off e2e job, so a recorded body would keep replaying the old shape. Same treatment for the open-window branch: the claim that reconciliation cannot produce one is now stated against the mechanism it rests on (RECONCILABLE_STATES, and SETTLED requiring window_closed), with the fix to apply if that ever stops holding.
…o end Review follow-ups. - The previous commit made TextOutput read self.color, but setup_logging never received the flag, so `polyswarm --no-color -v …` still emitted a green log prefix on a tty — the same half-honoured flag, one layer over. Threaded through, and the spec now states the flag's scope. - TestColorFlag pinned _paint, not the option: the flag travels through `formatters[output_format](color=color, …)` in the command group, which a formatter unit test cannot observe (specs/04 says so itself). Added a CliRunner test with color=True, which stops click stripping ANSI off the non-tty capture so the two runs differ only in the flag. - specs/03 claimed _paint means "a new helper cannot style unconditionally". It is a convention, not an enforcement point — reworded. - specs/05: the floor precondition needs the version read off the archive's own tree, because PEP 440 orders 4.2.0.dev1 < 4.2.0 and the SDK has a bumpversion dev part. Both files on origin/develop say 4.2.0 with no suffix. - Anchor the PolyScore assertion on the parsed line: 'PolyScore: 0.9' also matches 0.95, and the :.20f render is not the literal '0.9'.
Review follow-ups. - The colour test ran at default verbosity, so the level was WARNING and no record ever reached NamedColorFormatter: the log-prefix half of the fix was untested. Added the -v pair. - Both colour tests now mock `Polyswarm.search_hashes` (specs/04 Style 2) instead of replaying a cassette. The response content is irrelevant to whether the flag reaches the renderer, so a cassette would have to be recorded against a live stack for a test that never exercises the server — and borrowing another test's cassette coupled the two through the re-record path, since unittest orders methods alphabetically and this one sorts first. - `_render_styled`'s docstring said `color=True` was "passed only for emphasis". That was true before `_paint`; it is now load-bearing, and saying otherwise invites a future editor to drop it and silently disarm every colour assertion. Corrected in the docstring and in specs/03.
Review follow-ups. - _run_color_pair patched Polyswarm.search_hashes, which is CLI code, so it cut utils.parallel_executor_iterable_results out of the run — and the docstring cited it as "specs/04 Style 2", which defines that style as patching polyswarm_api.api.PolyswarmAPI.<method>. Now patches PolyswarmAPI.search: same assertions, correct seam, and the citation is true. - specs/03 and _paint's docstring both credited JSONOutput with honouring --no-color. It has no __init__ and emits plain JSON; PrettyJSONOutput is the one that reads the flag. The contrast is the stated rationale for _paint, so naming the wrong class undercut it.
Review follow-up. The text keyed on `valid_assertions and window_closed`; the colour keyed on `malicious_assertions and window_closed`. Two differently-shaped conditions that had to agree, held together only by the SDK guaranteeing malicious_assertions is a subset of valid_assertions (both filter on `mask`, one additionally on `verdict`). It is a subset today — verified — so there is no live defect, but a rendering rule in this repo should not depend on a property of another repo's resource class to stay coherent. Each branch now names its own paint function, so the count branch is the only one that can redden and the clause it renders is the one being coloured.
fix: key known-goodness on the instance state, not on the feed list
Review — Release 4.3.0The code changes themselves look correct and well covered. Everything below is about the release mechanics and spec bookkeeping this PR carries onto 1. This PR as it stands is not a release — no version bumpThe merge diff has
Merging as-is publishes nothing while the PR is titled Release 4.3.0. The Same for the TL;DR bullet "The SDK dependency floor is raised to polyswarm-api 4.3.0": the diff under review raises it to 2. Spec drift the moment #264 lands —
|
release: bump version to 4.3.0, floor the SDK at 4.3.0
|
Review — release 4.3.0 Gitflow is correct: Three things to fix before this merges. 1. The SDK floor bump is spec drift —
AGENTS.md: "Update the spec in the same PR as the code change; if a PR drifts from a spec, the spec is wrong until proven otherwise." Here the code and the spec disagree inside a single PR. The 2. The floor preconditions were verified for 4.2.0, not for 4.3.0
The PR body 3. Internal ticket ID in a commit that will land on
Minor The |
3 participants
TL;DR — changelog for 4.3.0
--no-coloris honoured end to end, including the log prefixes.Requires